allow overriding pytest functional arguments with posargs #1732

singuliere · 2019-02-08T10:50:24Z

It is useful to run a single test from the command line with, for instance:

tox -e ansibledevel-functional -- -k 'test_command_init_role[docker]' test/functional/test_command.py

The test/functional/ pytest argument is made a default used when there are no posargs instead of being hardcoded and always used. If all pytest invocations include test/functional/ there is no way for the user to restrict the tests being run to a single file.

PR Type

Feature Pull Request

It is useful to run a single test from the command line with, for instance: tox -e ansibledevel-functional -- -k 'test_command_init_role[docker]' test/functional/test_command.py The test/functional/ pytest argument is made a default used when there are no posargs instead of being hardcoded and always used. If all pytest invocations include test/functional/ there is no way for the user to restrict the tests being run to a single file. Signed-off-by: singuliere <[email protected]>

decentral1se

LGTM 👍 Thanks! I would hope to see a green CI 😅

singuliere · 2019-02-08T15:24:16Z

@lwm thanks for the quick review :-) I see you have a green ✔️ and @pilou- has a gray ✅ . Does that mean you have merge rights and @pilou- does not? Or is it unrelated?

webknjaz · 2019-02-08T15:56:15Z

I think green only means a member of org review, not merge rights.

webknjaz · 2019-02-10T01:21:33Z

tox.ini

@@ -30,7 +30,7 @@ extras =
    vagrant
 commands =
    unit: pytest test/unit/ --cov={toxinidir}/molecule/ --no-cov-on-fail {posargs}
-    functional: pytest test/functional/ {posargs}
+    functional: pytest {posargs:test/functional/}


There's no point in doing this since currently -- -k ... syntax works. With this, it'll match tests from unit test suite as well.

The reason why pytest has both -k and positional arguments is the same reason why this patch is proposed. For instance, it allows for tox -- test/test.py to run all test from a single test file. Achieving the same result with the -k option would be impractical.

I see why you'd want that and it's a natural wish.

Current tox.ini combines too much test modes in one section. I think it's an antipattern and should be improved.

Now, unit and functional factors correspond to running unit tests and functional tests respectively. But the proposed change advertises abusing that semantics which is far from being intuitive from the UX point of view.

I propose the following (thoughts, which I was having in mind for a while now):

separate these sections to make a clear distinction between their purposes

have a dedicated section for running whatever you want with pytest

[maybe] remove that tags plugin, I haven't heard about anyone using it, it complicates things a lot and it's probably possible to achieve the same result in other ways...

P.S. If you want, I can do (1) and (2) since I've got some experience with tox internals.

P.P.S. @ssbarnea has submitted an unfinished attempt to improve other aspects of tox config @
cb4ad30 via #1547 in past. This is also worth exploring...

But the proposed change advertises abusing that semantics which is far from being intuitive from the UX point of view.

I respectfully disagree. This is a very common and convenient pattern.

You've raised important points @webknjaz. If I read this correctly, your comment in #1732 (comment) shows us a more sustainable plan for our Tox usage. That's great and I would like to see that in a separate issue to share knowledge of what needs to be done and who is doing it.

For the purposes of this PR: this change might not be taking us in the direction of what you've desribed but it is making it more convenient to run specific tests for contributors in the right here and now. That's important and for that reason, I'd like to see this change merged as a 'low road' to making Tox easier to work with. WDYT?

@lwm

Feel free to overrule my judgment, for now. We can fix it later, after all.

OTOH after this change, it will be possible running unit tests from within functional factor which makes the existence of unit factor questionable.

P.S. on

is making it more convenient to run specific tests

it is already possible:

tox -e unit -- -k test_unit_smth tox -e functional -- -k test_functional_smth

and after this change it will be

tox -e functional -- -k test_unit_smth tox -e functional -- -k test_functional_smth tox -e functional -- test/unit/test_unit_thing.py::test_unit_smth

and if you apply the suggestion below (https://github.com/ansible/molecule/pull/1732/files#r257429626) if will look like

tox -e pytest -- -k test_unit_smth tox -e pytest -- -k test_functional_smth tox -e pytest -- test/unit/test_unit_thing.py::test_unit_smth tox -e pytest -- test/functional/test_func_thing.py::test_functional_smth

Feel free to overrule

Overruling is not productive behaviour.

Not if I explicitly allowed you to :)

In this case, it may be more productive as you need to keep going with improvements and I don't have any time involvement guarantees. I just wanted to share/document my vision but not block anyone.

duck-rh

LGTM

webknjaz · 2019-02-16T00:04:51Z

tox.ini

@@ -30,7 +30,7 @@ extras =
    vagrant
 commands =
    unit: pytest test/unit/ --cov={toxinidir}/molecule/ --no-cov-on-fail {posargs}
-    functional: pytest test/functional/ {posargs}
+    functional: pytest {posargs:test/functional/}
    lint: flake8
    lint: yamllint -s test/ molecule/



Suggested change

[testenv:pytest]

commands =

pytest {posargs}

singuliere · 2019-02-16T13:49:21Z

I apologize for proposing such a controversial modification. I would not want to waste any more volunteer / unpaid time from molecule contributors because of this.

decentral1se · 2019-02-16T15:20:31Z

🤒

webknjaz · 2019-02-16T20:35:28Z

@singuliere hey, why didn't you just accept the suggested change I made? It's just one click and it'd achieve exactly what you wanted...

decentral1se · 2019-02-17T11:42:50Z

@webknjaz, I can only speculate it is because you 1) Pushed a number of unrelated of concerns onto this contributor - #1732 (comment) 2) used negative language such as "there is no point in doing this" - #1732 (comment) and "abusing that semantics" - #1732 (comment).

Hence, this reaction. This is not the first time a contributor has reacted negatively to a review of yours (please do not ask me to reference). I recommend that you reconsider your choice of language and try to consider and prioritise volunteer contributor capacity/time/motivation when making your reviews. We all want quality but there are other considerations ...

singuliere force-pushed the tox-pyttest-functional-posargs branch from a7aff6e to 6ce1be2 Compare February 8, 2019 10:50

singuliere changed the title ~~allow overriding functional arguments with posargs~~ allow overriding pytest functional arguments with posargs Feb 8, 2019

singuliere force-pushed the tox-pyttest-functional-posargs branch from 6ce1be2 to 7e97e42 Compare February 8, 2019 10:52

singuliere mentioned this pull request Feb 8, 2019

Compatibility with Ansible devel (2.8) broken #1727

Closed

decentral1se approved these changes Feb 8, 2019

View reviewed changes

pilou- approved these changes Feb 8, 2019

View reviewed changes

webknjaz reviewed Feb 10, 2019

View reviewed changes

duck-rh approved these changes Feb 11, 2019

View reviewed changes

webknjaz reviewed Feb 16, 2019

View reviewed changes

singuliere closed this Feb 16, 2019

This was referenced Feb 21, 2019

tox does not support tags out of the box #1764

Closed

Allow to pass extra hosts to the inventory #1748

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

allow overriding pytest functional arguments with posargs #1732

allow overriding pytest functional arguments with posargs #1732

singuliere commented Feb 8, 2019 •

edited

Loading

decentral1se left a comment

singuliere commented Feb 8, 2019

webknjaz commented Feb 8, 2019

webknjaz Feb 10, 2019

singuliere Feb 10, 2019

webknjaz Feb 11, 2019

webknjaz Feb 11, 2019

singuliere Feb 11, 2019

decentral1se Feb 15, 2019

webknjaz Feb 16, 2019

decentral1se Feb 17, 2019

webknjaz Feb 17, 2019

duck-rh left a comment

webknjaz Feb 16, 2019

singuliere commented Feb 16, 2019

decentral1se commented Feb 16, 2019

webknjaz commented Feb 16, 2019

decentral1se commented Feb 17, 2019 •

edited

Loading

+ [testenv:pytest]
+ commands =
+     pytest {posargs}

allow overriding pytest functional arguments with posargs #1732

allow overriding pytest functional arguments with posargs #1732

Conversation

singuliere commented Feb 8, 2019 • edited Loading

PR Type

decentral1se left a comment

Choose a reason for hiding this comment

singuliere commented Feb 8, 2019

webknjaz commented Feb 8, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

duck-rh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

singuliere commented Feb 16, 2019

decentral1se commented Feb 16, 2019

webknjaz commented Feb 16, 2019

decentral1se commented Feb 17, 2019 • edited Loading

singuliere commented Feb 8, 2019 •

edited

Loading

decentral1se commented Feb 17, 2019 •

edited

Loading